Skip to content
This repository has been archived by the owner on Dec 1, 2023. It is now read-only.

Proxy nile node unknown options to starknet-devnet #342

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

ericnordelo
Copy link
Member

@ericnordelo ericnordelo commented Jan 18, 2023

Fixes #293

This PR proposes to make available to the nile node interface all the options that straknet-devnet accepts by default.

This enables forking, but also the ability to specify the initial balances, the number of predeployed accounts, the gas price to be used, the account class to be used, etc...

Ex:

nile node --fork-network alpha-mainnet --fork-block 4 --gas-price 1234

Previous lite mode flag was labeled --lite_mode, and now it must be named --lite-mode, so this is a small breaking change.

Copy link
Contributor

@andrew-fleming andrew-fleming left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great work, Eric! I left a couple questions :)

Comment on lines 291 to 293
NOTE: Accepts all the options that the starknet-devnet node accepts by default. Check the {devnet-doc} for a full
list of them.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This might be a bit overkill. We're basically saying the same thing below with Starknet Devnet Options. I'm thinking we can remove the note here. What do you think?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added it, removed it, and added it again a couple of times thinking about the design. I'm not too comfortable 100 percent with either of the versions, because I wanted to let it as clear as possible for the user that he can use these arguments from starknet-devnet, but not adding all of them here. At the same time, I feel the user can check the command doc in a quick glance and miss this "Starknet Devnet Options".

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm ok with removing the note, but I still have the feeling the user can miss this.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Haha yeah, I see the dilemma. If we remove host and port, there's much less noise around the node section. I think it's enough, no? Here's a crude example of how I'm seeing it:

node

----
nile node
----
Run a local [starknet-devnet](https://github.com/Shard-Labs/starknet-devnet/) node.

Starknet Devnet Options
This command exposes...

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. I think that should be enough indeed.

def node(host="127.0.0.1", port=5050, seed=None, lite_mode=False):
def node(host="127.0.0.1", port=5050, node_args=None):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since host and port are passed to the devnet in the the same way as the other devnet options (and the defaults match), can't we just stick with node_args as a single catch-all param?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just wanted to let the defaults in our control, but if they match I agree it makes sense to remove them.

Copy link
Member Author

@ericnordelo ericnordelo Jan 19, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On second thought. I will let the host and port options as a commodity if you agree because we are using them here:

        if host == "127.0.0.1":
            network = "localhost"
        else:
            network = host
        gateway_url = f"http://{host}:{port}/"
        if DEFAULT_GATEWAYS.get(network) != gateway_url:
            write_node_json(network, gateway_url)

And if we remove them, I would need to parse the node_args to get the values anyway (is easier to catch them separately as we are currently doing).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ahh right! Yeah, leaving them in sounds good to me. Depending on how the Nile config turns out down the road, maybe we can revisit this.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good.

Comment on lines +356 to +357
class NodeCommand(click.Command):
"""Command wrapper to override the default help message."""
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

Copy link
Contributor

@andrew-fleming andrew-fleming left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to go!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support mainnet forking
2 participants